fix: Graph now notify updater when cycles are formed or broken#1647
Merged
Conversation
Previously, the updater only considered nodes that were changed from an edge insertion or deletion. However, an edge insertion/deletion can also change the looped status of a disconnected node. For example, if we have a -> b and b -> a, and we remove a -> b, only b is marked as changed. This is incorrect, since a changed indirectly, since it is no longer looped. Thus, the graph implementations now mark any nodes whose SCC change from singleton to multinode (looped) and vice-versa.
Christopher-Chianelli
temporarily deployed
to
internal
GitHub Actions
Inactive
triceo
approved these changes
Jun 12, 2025
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the graph implementation to notify the updater when nodes’ looped (cycle) status changes by propagating a shared BitSet through graph operations and recording SCC status flips.
- Method signatures for
commitChanges,addEdge, andremoveEdgenow accept aBitSet changedto mark nodes whose cycle membership toggles. - Tests in
VariableListenerSupportTestare updated to pass the newchangedparameter; a newDefaultTopologicalGraphTestis added. DefaultTopologicalOrderGraphtracks and flags nodes whose SCC size moves between singleton and multi-node loops, and adds atoStringfor debugging.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
VariableListenerSupportTest.java |
Updated overridden addEdge/removeEdge to include BitSet changed. |
DefaultTopologicalGraphTest.java |
New test class for DefaultTopologicalOrderGraph. |
TopologicalOrderGraph.java |
Changed signatures of commitChanges, addEdge, removeEdge to take BitSet changed. |
DefaultVariableReferenceGraph.java |
Pass the shared changed BitSet into graph operations and commit. |
DefaultTopologicalOrderGraph.java |
Added isNodeInLoopedComponent, track SCC status flips, and debug toString(). |
Comments suppressed due to low confidence (2)
core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/TopologicalOrderGraph.java:13
- The new
changedparameter isn’t documented in the Javadoc. Please add an@param changedexplaining that it is a BitSet used to collect nodes whose looped status has changed.
void commitChanges(BitSet changed);
core/src/test/java/ai/timefold/solver/core/impl/domain/variable/declarative/DefaultTopologicalGraphTest.java:5
- [nitpick] The test class name omits "Order" and doesn’t match the tested
DefaultTopologicalOrderGraph. Consider renaming this toDefaultTopologicalOrderGraphTestfor consistency.
public class DefaultTopologicalGraphTest extends AbstractTopologicalGraphTest<DefaultTopologicalOrderGraph> {
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously, the updater only considered nodes that were changed from an edge insertion or deletion. However, an edge insertion/deletion can also change the looped status of a disconnected node. For example, if we have a -> b and b -> a, and we
remove a -> b, only b is marked as changed. This is incorrect, since a changed indirectly, since it is no longer looped. Thus, the graph implementations now mark any nodes whose SCC change from singleton to multinode (looped) and vice-versa.